Bug 1955969: - Makes port names unique. #181

adduarte · 2021-05-05T22:54:08Z

The bugzilla in question is rooted on attempting to create
ports with the same name.

This patch uses the subnet and netids as name suffixes for ports.

Also limits the length of the port name to no more than 255 characters, which is in line with what the current neutron db schema can tolerate.

https://issues.redhat.com/browse/OCPBUGSM-28624

openshift-ci-robot · 2021-05-05T22:54:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from adduarte after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/cloud/openstack/clients/machineservice.go

mandre

Seems reasonable.

pkg/cloud/openstack/clients/machineservice.go

adduarte · 2021-05-06T19:47:39Z

/test e2e-openstack

adduarte · 2021-05-06T19:49:30Z

/test e2e-openstack-ipi

openshift-ci · 2021-05-06T19:49:31Z

@adduarte: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test e2e-openstack
/test e2e-upgrade
/test gofmt
/test golint
/test govet
/test images
/test unit

Use /test all to run the following jobs:

pull-ci-openshift-cluster-api-provider-openstack-master-e2e-openstack
pull-ci-openshift-cluster-api-provider-openstack-master-gofmt
pull-ci-openshift-cluster-api-provider-openstack-master-golint
pull-ci-openshift-cluster-api-provider-openstack-master-govet
pull-ci-openshift-cluster-api-provider-openstack-master-images
pull-ci-openshift-cluster-api-provider-openstack-master-unit

Details

In response to this:

/test e2e-openstack-ipi

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/cloud/openstack/clients/machineservice.go

adduarte · 2021-05-06T23:37:29Z

/test e2e-openstack

adduarte · 2021-05-06T23:40:14Z

/hold

adduarte · 2021-05-07T00:56:41Z

/hold cancel

adduarte · 2021-05-07T02:40:03Z

/hold

adduarte · 2021-05-07T02:57:42Z

/test e2e-openstack

adduarte · 2021-05-07T03:50:27Z

/hold cancel

openshift-ci · 2021-05-07T03:57:33Z

@adduarte: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-05-07T03:58:06Z

@adduarte: This pull request references Bugzilla bug 1955969, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (juriarte@redhat.com), skipping review request.

Details

In response to this:

Bug 1955969: - Makes port names unique.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/cloud/openstack/clients/machineservice.go

adduarte · 2021-05-07T04:00:11Z

This is ready to merge once we get a green on ci/prow/e2e-openstack

openshift-ci · 2021-05-07T04:04:40Z

@adduarte: This pull request references Bugzilla bug 1955969, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (juriarte@redhat.com), skipping review request.

Details

In response to this:

Bug 1955969: - Makes port names unique.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pierreprinetti · 2021-05-07T05:17:21Z

Do you plan on changing port names in Terraform as well?

Also this requires a review by the Kuryr folk:
/cc @MaysaMacedo

openshift-ci · 2021-05-07T05:17:22Z

@pierreprinetti: GitHub didn't allow me to request PR reviews from the following users: maysamacedo.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

don’t forget to change the name in Terraform as well for consistency.

Also this requires a review by the Kuryr folk:
/cc @MaysaMacedo

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mandre · 2021-05-07T06:27:47Z

pkg/cloud/openstack/clients/machineservice.go

+		portName = name
+	}
+	if len(portName) > PortNameMaxSize {
+		portName = portName[len(portName)-PortNameMaxSize:]


The logic for shortening the the port name in case it's too long takes the last 255 chars, I would have personally kept the first chars since the name is supposed to be for humans. Not that it matters, I expect we never end up in a situation where the port name is that long anyway...

mandre · 2021-05-07T06:46:27Z

@adduarte I hope you don't mind, I changed the commit message. Your commit still had the WIP tag in its subject.

EmilienM · 2021-05-07T07:15:58Z

/hold
until someone explains correctly why we don't comment the code here.
I requested that we comment the 255 number to explain why we made that decision.
Having it in the commit message is not enough. It is good practice to comment directly in the code.

Thank you.

The bugzilla in question is rooted on attempting to create ports with the same name. This patch uses the subnet and netids as name suffixes for ports. Also limits the length of the port name to no more than 255 characters, which is in line with what the current neutron db schema can tolerate.

mandre · 2021-05-07T07:27:40Z

/hold
until someone explains correctly why we don't comment the code here.
I requested that we comment the 255 number to explain why we made that decision.
Having it in the commit message is not enough. It is good practice to comment directly in the code.

You're entirely correct, I've made the requested change.
/hold cancel

dulek · 2021-05-07T07:36:06Z

Please note that backporting this to 4.7 will break CNO's ability to deploy an API LB through Octavia, effectively breaking installation with Kuryr. It'd require some more effort to make it work.

mandre · 2021-05-07T11:54:19Z

/lgtm
/approve

openshift-ci · 2021-05-07T11:54:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandre

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mandre]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2021-05-07T11:55:36Z

@adduarte: All pull requests linked via external trackers have merged:

openshift/cluster-api-provider-openstack#181

Bugzilla bug 1955969 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1955969: - Makes port names unique.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

* Make yaml lint happy * Add filter key for networks

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 5, 2021

openshift-ci-robot requested review from EmilienM and mandre May 5, 2021 22:54

adduarte commented May 5, 2021

View reviewed changes

pkg/cloud/openstack/clients/machineservice.go Outdated Show resolved Hide resolved

adduarte force-pushed the BZ_1955969 branch from 91c3bf6 to a539c9a Compare May 6, 2021 13:58

mandre reviewed May 6, 2021

View reviewed changes

pkg/cloud/openstack/clients/machineservice.go Show resolved Hide resolved

adduarte force-pushed the BZ_1955969 branch from a539c9a to 957fa6a Compare May 6, 2021 21:24

adduarte commented May 6, 2021

View reviewed changes

pkg/cloud/openstack/clients/machineservice.go Show resolved Hide resolved

adduarte force-pushed the BZ_1955969 branch from 957fa6a to c367017 Compare May 6, 2021 21:58

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2021

adduarte force-pushed the BZ_1955969 branch 3 times, most recently from 05fd220 to 200009b Compare May 7, 2021 00:19

adduarte changed the title ~~[WIP] Bugzilla 1955969 - Makes port names unique.~~ Bugzilla 1955969 - Makes port names unique. May 7, 2021

openshift-ci bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 7, 2021

adduarte changed the title ~~Bugzilla 1955969 - Makes port names unique.~~ Bug 1955969 - Makes port names unique. May 7, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2021

adduarte requested a review from mandre May 7, 2021 03:54

adduarte changed the title ~~Bug 1955969 - Makes port names unique.~~ Bug 1955969: - Makes port names unique. May 7, 2021

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 7, 2021

EmilienM reviewed May 7, 2021

View reviewed changes

pkg/cloud/openstack/clients/machineservice.go Show resolved Hide resolved

mandre reviewed May 7, 2021

View reviewed changes

mandre force-pushed the BZ_1955969 branch from 200009b to 9b68df9 Compare May 7, 2021 06:44

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2021

mandre force-pushed the BZ_1955969 branch from 9b68df9 to edff8e5 Compare May 7, 2021 07:21

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2021

openshift-ci bot assigned mandre May 7, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2021

openshift-merge-robot merged commit 03612d3 into openshift:master May 7, 2021

pierreprinetti deleted the BZ_1955969 branch May 7, 2021 14:48

EmilienM mentioned this pull request Jan 31, 2023

Manual rebase on upstream #252

Closed

pierreprinetti pushed a commit to shiftstack/cluster-api-provider-openstack that referenced this pull request Apr 22, 2024

Add the filter key to the network's CRD entry (openshift#181)

ea40b6b

* Make yaml lint happy * Add filter key for networks

Bug 1955969: - Makes port names unique. #181

Bug 1955969: - Makes port names unique. #181

Uh oh!

Conversation

adduarte commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 5, 2021

Uh oh!

Uh oh!

mandre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adduarte commented May 6, 2021

Uh oh!

adduarte commented May 6, 2021

Uh oh!

openshift-ci bot commented May 6, 2021

Uh oh!

Uh oh!

adduarte commented May 6, 2021

Uh oh!

adduarte commented May 6, 2021

Uh oh!

adduarte commented May 7, 2021

Uh oh!

adduarte commented May 7, 2021

Uh oh!

adduarte commented May 7, 2021

Uh oh!

adduarte commented May 7, 2021

Uh oh!

openshift-ci bot commented May 7, 2021

Uh oh!

openshift-ci bot commented May 7, 2021

Uh oh!

Uh oh!

adduarte commented May 7, 2021

Uh oh!

openshift-ci bot commented May 7, 2021

Uh oh!

pierreprinetti commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented May 7, 2021

Uh oh!

mandre May 7, 2021

Choose a reason for hiding this comment

Uh oh!

mandre commented May 7, 2021

Uh oh!

EmilienM commented May 7, 2021

Uh oh!

mandre commented May 7, 2021

Uh oh!

dulek commented May 7, 2021

Uh oh!

mandre commented May 7, 2021

Uh oh!

openshift-ci bot commented May 7, 2021

Uh oh!

openshift-ci bot commented May 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

adduarte commented May 5, 2021 •

edited

Loading

pierreprinetti commented May 7, 2021 •

edited

Loading